feat: add std.PrevRealm #667

albttx · 2023-03-28T19:00:17Z

Description

Currently the p/demo/grc/grc20 package and r/demo/foo20 contract use std.GetOrigCaller for checking the owner, this could lead to potential security issues (eg: phishing).

func (t *userToken) Transfer(to std.Address, amount uint64) error {
	owner := std.GetOrigCaller() // vulnerable
	return t.admin.transfer(owner, to, amount)
}

flowchart LR
   A[user1.gno] --> B("realm1.gno (caller is user1.gno)")
   B --> |"call unsafe package"| C[p/malicious]
   C --> |"call foo20.Transfer and steal tokens"| D("foo20.gno")

By using std.GetCaller, we fix this situation.

func (t *userToken) Transfer(to std.Address, amount uint64) error {
	owner := std.GetCaller()
	return t.admin.transfer(owner, to, amount)
}

NOTE: The changes in the grc20 packages and foo20 will be executed on another PR to keep this one small

How it's works

Add std.GetCaller which return the latest caller.

This should be mostly used on grc20 package rather than std.GetOrigCaller

How it's works

GetCaller already return the Caller, When the caller is a Realm, return the realm, if not, return the calling user.

Examples

User calm realm, that call realms

flowchart LR
    A[user1.gno] --> B("realm1.gno (caller is user1.gno)")
    B --> C("r/realm2 (caller is realm1.gno)")
    C --> D("r/realm3 (caller is realm2.gno)")

User call Realm that call only packages

flowchart LR
    A[user1.gno] --> B(realm.gno)
    B --> C("p/foo (caller is user.gno)")
    C --> D("p/bar (caller is user.gno)")

User call Realm that call packages and realm

Noticed that on r/bar the caller is realm.gno

flowchart LR
    A[user1.gno] --> B(realm.gno)
    B --> C("p/foo (caller is user.gno)")
    C --> D("r/bar (caller is realm.gno)")

How has this been tested?

see tests...

go test -v ./gnovm/tests/ -run "/zrealm_crossrealm11.gno"
go test -v ./gnovm/tests/ -run "/zrealm_crossrealm_exploit1_stdlibs"

grepsuzette · 2023-03-29T02:07:35Z

I like this.
Maybe it should be called GetLastCaller().

tbruyelle · 2023-03-29T08:51:41Z

I like this. Maybe it should be called GetLastCaller().

That makes sense since we have also GetCallerAt(position).

But on the other hand, I'm wondering if having all these methods to access to the callers stack is really useful. Why a contract would need to access the nth caller ? Sounds like an open-bar for security breaches no ?

r3v4s · 2023-03-30T06:48:51Z

We do definitely need this kind of stuff.

Could add TestSetCaller?? (just like TestSetOrigCaller)
It might be useful when testing.

https://github.com/gnolang/gno/blob/master/tests/imports.go#L537

albttx · 2023-03-30T12:55:16Z

Maybe it should be called GetLastCaller().

I get your point, but it's semantically wrong i believe.

user.gno -> daoX -> foo20

user.gno is the GetOrigCaller.
for daoX, user.gno is the Caller
for foo20, daoX is the Caller, the "LastCaller" would be user.gno logically, (user.gno is still the GetOrigCaller of course)

It would made sense if the function was GetLast{Realm,Package}

@grepsuzette wdyt ?

albttx · 2023-03-30T13:56:10Z

Could add TestSetCaller?? (just like TestSetOrigCaller)

This is more complicate than it's look, TestSetOrigCaller just force the (*Machine).Context.OrigCaller,

GetCaller() is based on frame[i-1].LastPackage.GetPkgAddr(), which use DerivePackageAddr() on the pkgName, so if i force and edit of the frame package name, i might have side effets!

Maybe we should consider adding the Caller directly in the Context ? But the context is attached to the Machine, which is kind of annoying...
Or we can add it into the Frame?

r3v4s · 2023-03-30T14:37:07Z

Could add TestSetCaller?? (just like TestSetOrigCaller)

This is more complicate than it's look, TestSetOrigCaller just force the (*Machine).Context.OrigCaller,

GetCaller() is based on frame[i-1].LastPackage.GetPkgAddr(), which use DerivePackageAddr() on the pkgName, so if i force and edit of the frame package name, i might have side effets!

Maybe we should consider adding the Caller directly in the Context ? But the context is attached to the Machine, which is kind of annoying... Or we can add it into the Frame?

Hmmm.. I think we better leave gno context structure as it as for now, and find ways to spoof or mock the frame with least side effetcs

As you said touching context will be super annoying and likely to have more side effects

piux2 · 2023-03-30T15:43:48Z

I agree; changing context dynamically in the call path makes the contracts hard to audit and opens doors for security issues.

In typical use cases, OrigCaller refers to the user account that signs the original message, not a contract on the call path. It equals EOA in Solidity.

In the file test, OrigCaller is the main package. It simplifies the testing process since we don't need to sign the message to initiate a call to contract from EOA. TestSetOrigCaller is only used to replace the main package for the admin user for testing purposes.

moul · 2023-03-31T13:27:45Z

Please share your thoughts on PR #683.

tbruyelle · 2023-03-31T14:34:54Z

Question: What do we want as ouput of std.GetCaller when it's executed by a package?

That's an interesting question, can a package be considered as a caller or not ?

I had the similar question for #495 for std.AssertOriginCall function. Basically, should std.AssertOriginCall panic if called inside a package called by a realm ? If yes, that means we consider the package as a caller, and as it's not the origin caller, the function panics.

grepsuzette · 2023-03-31T18:19:11Z

Perhaps the name is not very important for now.

@albttx I will try to rephrase your use case.

There is a user Bob, Bob represents a DAO on GNO, this DAO exists through a realm. Last week the DAO decided to emit GRC20 tokens and to airdrop them to 100 random users of r/boards.

You're saying that in the code implementing this last sentence, the call std.GetOrigCaller() should play no part and should not be called. Correct?
But that std.GetOrigCaller() is of course useful in general. Correct?

If that's correct, I'm wondering if we can think of scenarios where std.GetCallerAt() would be useful.
I'm of course trying to make an opinion for #683.

albttx · 2023-04-01T08:55:53Z

Hey @grepsuzette, no, here is the current situation

See here the grc20 code:

The user_token check the GetOrigCaller

a user.gno, the only member of the DAO (to make it simple)
gno.land/r/the-corp/dao a realm for a DAO, (wallet the-corp-dao.gno)
gno.land/r/the-corp/foo20 a $FOO GRC20 realm

Some infos:

The DAO have a treasury of 1_000_000 $FOO
user.gno have 0 $FOO

So now let's say, user.gno want to spend 200_000$FOO to grepsuzette.gno via the DAO

So, the tx will look like:

user.gno -> gno.land/r/the-corp/dao -> gno.land/r/the-corp/foo20

So user.gno call the DAO realm, (GetOrigCaller == user.gno)
DAO call FOO.Transfer() GetOrigCaller == user.gno, so we'll fail on insufisent fund, because the caller is user.gno where it shoud be the-corp-dao.gno

This PR adding std.GetCaller fix this issue.

moul

Blocking until we choose the best direction for this in #683.

anarcher · 2023-04-02T12:12:21Z

Personally, I like the name std.CallerAddress() (in case we need std.Caller() -> Caller (sturct or interface) later... :-))

moul · 2023-04-02T20:39:46Z

std.GetCaller().Addr()?

gnovm/tests/files/zrealm_crossrealm11.gno

albttx · 2023-04-05T10:29:43Z

linked to #634

examples/gno.land/r/demo/tests/tests.gno

gnovm/stdlibs/stdlibs.go

jaekwon · 2023-06-02T12:38:31Z

On another note, I suggest we revisit the API design and propose an alternative: std.RealmAt(X) as suggested by Jae. Here, std.RealmAt(0) would return r/boards, and std.RealmAt(1) would return user1.gno.

but what's the reason for needing that? first I would get a good use-case that isn't addressed by existing functions, and make sure this use-case is definitely something we want to support, rather than an anti-pattern.

jaekwon · 2023-06-02T12:39:40Z

reading it once more now.

gnovm/stdlibs/std/frame.gno

examples/gno.land/p/demo/tests/subtests/subtests.gno

gnovm/stdlibs/stdlibs.go

jaekwon · 2023-06-02T13:04:54Z

gnovm/stdlibs/stdlibs.go

+
+				// Empty the pkgPath if we return a user
+				if ctx.OrigCaller == lastCaller {
+					lastPkgPath = ""


i don't understand why it needs to be empty. what happens without this line?
Maybe it's solved by the if-else chain i suggested above. it seemed like it was unnecessarily setting lastPkgPath before.

std.Realm return a Realm{}

type Realm struct { Addr crypto.Bech32Address PkgPath string }

in the case we return a user, we want PkgPath to be empty.

Without this line, we could return a Realm containing a user address and gno.land/r/demo/xxx which is the "Current realm".

This condition won't be present in the next function std.Realm

For accounts, it's a good idea to have an empty variable. However, if default realm implementation is desired later, it could change to gno.land/r/moul. What is the current value before erasing it?

I like using if PkgPath != "" for distinction, similar to if err != nil. We can update the method or choose a new variable when accounts have a path.

Waiting for your response, but currently inclined to maintain the current behavior.

Hi @moul I'm not sure i understand your question, but here is a better explanation

user1.gno -> gno.land/r/moul/sayhi

The Frames will be

gno.land/r/moul/sayhi

user1.gno

so when we will finish the iteration loop, lastCaller will be user1.gno, and pkgPath will be gno.land/r/moul/sayhi, ( frame[i].LastPackage.PkgPath)

So we use if ctx.OrigCaller == lastCaller to empty it

Is it more clear ?

I was asking what was the raw value before erasing it. Checking if it makes sense to be kept somehow.

jaekwon

Really close to final; just have the comments written so far.
If nobody objects to new changes, feel free to merge if all comments are addressed.

r3v4s · 2023-06-13T01:23:43Z

Just wanna check

PrevRealm is like eth's msg.sender
GetOrigCaller is like eths' tx.origin

Did I understood correctly?

r3v4s · 2023-06-13T07:50:13Z

It works perfectly fine for actual gno running with gnoland, and interacting with gnokey

However, because gno test uses its own context, PrevRealm() doesn't work on test case.

gno test -root-dir ~/gno -verbose=true ./
Test OrigCaller: g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm

=== RUN   TestFunc
outer_std.GetOrigCaller(): g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm
outer_std.PrevRealm(): struct{("g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm" std.Address),("" string)}
called OuterToInner()

calling inner.Inner() from outer
inner_std.GetOrigCaller(): g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm
inner_std.PrevRealm(): struct{("g1wymu47drhr0kuq2098m792lytgtj2nyx77yrsm" std.Address),("" string)}
called Inner()

FINISH OuterToInner()
--- PASS: TestFunc (0.00s)
ok      ./. 	0.49s

So I'v made little helper #891

Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com> Co-authored-by: Thomas Bruyelle <thomasbruyelle@hey.com> Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

update (missing part of) grc721 package implementation to use `std.PrevRealm()` not `std.GetOrigCaller()` ## related #667 implementation of `std.PrevRealm()` #895 update grc20, 721 to use `std.PrevRealm()` <details><summary>Checklists...</summary> ## Contributors Checklist - [x] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](../.benchmarks/README.md). </details> Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

update (missing part of) grc721 package implementation to use `std.PrevRealm()` not `std.GetOrigCaller()` ## related gnolang#667 implementation of `std.PrevRealm()` gnolang#895 update grc20, 721 to use `std.PrevRealm()` <details><summary>Checklists...</summary> ## Contributors Checklist - [x] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests - [ ] Added new benchmarks to [generated graphs](https://gnoland.github.io/benchmarks), if any. More info [here](../.benchmarks/README.md). </details> Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

@albttx

# Description Thanks to @albttx, we now have `std.PrevRealm` #667 However it seems like doesn't work on testcase(that can be run by `gno test` command) as what we expected. > `gno test` uses its own Context which makes few other functions (like GetRealmPath) doesn't work neither. So I made little helper very similar to `std.TestSetOrigCaller` --- ## Discussion Need `std.TestSetOrigCaller` takes single parameter and two different type can be passed 1. If `std.Address` type is passed, -> TestSetPrevRealm will take it as user address => return user address not realm. 2. If `string` type is passed, -> TestSetPrevRealm will take it as pkg_path => return pkg address(using bech32) and pkg_path > Since string parameter is being used without any verification in this pr, How about reusing verification logic from here ??https://github.com/gnolang/gno/blob/408fc68d4b3c189dbc6a608c590a86c661ae232a/tm2/pkg/std/memfile.go#L33-L68 ## Contributors Checklist - [x] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests ## Maintainers Checklist - [x] Checked that the author followed the guidelines in `CONTRIBUTING.md` - [x] Checked the conventional-commit (especially PR title and verb, presence of `BREAKING CHANGE:` in the body) - [x] Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

@albttx

# Description Thanks to @albttx, we now have `std.PrevRealm` gnolang#667 However it seems like doesn't work on testcase(that can be run by `gno test` command) as what we expected. > `gno test` uses its own Context which makes few other functions (like GetRealmPath) doesn't work neither. So I made little helper very similar to `std.TestSetOrigCaller` --- ## Discussion Need `std.TestSetOrigCaller` takes single parameter and two different type can be passed 1. If `std.Address` type is passed, -> TestSetPrevRealm will take it as user address => return user address not realm. 2. If `string` type is passed, -> TestSetPrevRealm will take it as pkg_path => return pkg address(using bech32) and pkg_path > Since string parameter is being used without any verification in this pr, How about reusing verification logic from here ??https://github.com/gnolang/gno/blob/408fc68d4b3c189dbc6a608c590a86c661ae232a/tm2/pkg/std/memfile.go#L33-L68 ## Contributors Checklist - [x] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests ## Maintainers Checklist - [x] Checked that the author followed the guidelines in `CONTRIBUTING.md` - [x] Checked the conventional-commit (especially PR title and verb, presence of `BREAKING CHANGE:` in the body) - [x] Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

@albttx

# Description Thanks to @albttx, we now have `std.PrevRealm` gnolang#667 However it seems like doesn't work on testcase(that can be run by `gno test` command) as what we expected. > `gno test` uses its own Context which makes few other functions (like GetRealmPath) doesn't work neither. So I made little helper very similar to `std.TestSetOrigCaller` --- ## Discussion Need `std.TestSetOrigCaller` takes single parameter and two different type can be passed 1. If `std.Address` type is passed, -> TestSetPrevRealm will take it as user address => return user address not realm. 2. If `string` type is passed, -> TestSetPrevRealm will take it as pkg_path => return pkg address(using bech32) and pkg_path > Since string parameter is being used without any verification in this pr, How about reusing verification logic from here ??https://github.com/gnolang/gno/blob/408fc68d4b3c189dbc6a608c590a86c661ae232a/tm2/pkg/std/memfile.go#L33-L68 ## Contributors Checklist - [x] Added new tests, or not needed, or not feasible - [ ] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests ## Maintainers Checklist - [x] Checked that the author followed the guidelines in `CONTRIBUTING.md` - [x] Checked the conventional-commit (especially PR title and verb, presence of `BREAKING CHANGE:` in the body) - [x] Ensured that this PR is not a significant change or confirmed that the review/consideration process was appropriate for the change

albttx requested a review from a team as a code owner March 28, 2023 19:00

albttx mentioned this pull request Mar 28, 2023

Draft: chore: std.GetCaller #666

Closed

albttx self-assigned this Mar 28, 2023

albttx added 🌱 feature New update to Gno investigating This behavior is still being tested out labels Mar 28, 2023

moul changed the title ~~Draft: chore: std.GetCaller~~ feat: add std.GetCaller Mar 28, 2023

moul marked this pull request as draft March 28, 2023 22:13

moul mentioned this pull request Mar 31, 2023

RFC Proposal for a new stack/frames management #683

Open

moul requested changes Apr 1, 2023

View reviewed changes

albttx force-pushed the albttx/get-caller-bis branch from 7eae128 to a70e880 Compare April 3, 2023 18:24

albttx commented Apr 3, 2023

View reviewed changes

gnovm/tests/files/zrealm_crossrealm11.gno Outdated Show resolved Hide resolved

albttx force-pushed the albttx/get-caller-bis branch from a70e880 to 1b628e6 Compare April 5, 2023 08:56

albttx mentioned this pull request Apr 5, 2023

chore(tests): different address on test caller #662

Merged

jaekwon reviewed Apr 18, 2023

View reviewed changes

examples/gno.land/r/demo/tests/tests.gno Outdated Show resolved Hide resolved

moul mentioned this pull request Apr 19, 2023

META contract-contract interactions on a single chain #757

Open

albttx commented Apr 20, 2023

View reviewed changes

gnovm/stdlibs/stdlibs.go Outdated Show resolved Hide resolved

jaekwon reviewed Jun 2, 2023

View reviewed changes

gnovm/stdlibs/std/frame.gno Outdated Show resolved Hide resolved

jaekwon reviewed Jun 2, 2023

View reviewed changes

examples/gno.land/p/demo/tests/subtests/subtests.gno Show resolved Hide resolved

jaekwon reviewed Jun 2, 2023

View reviewed changes

gnovm/stdlibs/stdlibs.go Outdated Show resolved Hide resolved

jaekwon reviewed Jun 2, 2023

View reviewed changes

jaekwon requested changes Jun 2, 2023

View reviewed changes

r3v4s mentioned this pull request Jun 5, 2023

fix: minting additional GNOTs #393

Closed

albttx added 3 commits June 6, 2023 16:18

chore: update condition to if-else for readability

505a5c6

chore: export Addr et PkgPath as methods

bb0d553

fix: stdshim realm

fb9245b

r3v4s mentioned this pull request Jun 7, 2023

feat(stdlibs/std)!: namespace minted coins with realm path #875

Merged

9 tasks

moul merged commit 408fc68 into gnolang:master Jun 12, 2023

This was referenced Jun 13, 2023

feat: std.TestSetPrevRealm #890

Closed

feat: std.TestSetPrevRealm #891

Merged

This was referenced Jun 14, 2023

feat: std.CurrentRealm #893

Merged

chore: use std.PrevRealm() in grc20 and grc721 #895

Merged

tbruyelle mentioned this pull request Jun 14, 2023

fix(gnovm): PrevRealm in _test files #896

Merged

9 tasks

r3v4s mentioned this pull request Jun 19, 2023

[WIP]feat: Protocol to support contract-contract interaction #473

Draft

r3v4s mentioned this pull request Jul 24, 2023

chore: use std.PrevRealm() in grc721 package #992

Merged

7 tasks

moul mentioned this pull request Apr 30, 2024

feat(gnovm, tm2): implement event emission with std.Emit #1653

Merged

thehowl mentioned this pull request Oct 31, 2024

META - splitting and refactoring std #1475

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add std.PrevRealm #667

feat: add std.PrevRealm #667

albttx commented Mar 28, 2023 •

edited

Loading

grepsuzette commented Mar 29, 2023

tbruyelle commented Mar 29, 2023 •

edited

Loading

r3v4s commented Mar 30, 2023

albttx commented Mar 30, 2023

albttx commented Mar 30, 2023

r3v4s commented Mar 30, 2023

piux2 commented Mar 30, 2023

moul commented Mar 31, 2023

tbruyelle commented Mar 31, 2023 •

edited

Loading

grepsuzette commented Mar 31, 2023

albttx commented Apr 1, 2023

moul left a comment

anarcher commented Apr 2, 2023

moul commented Apr 2, 2023

albttx commented Apr 5, 2023

jaekwon commented Jun 2, 2023

jaekwon commented Jun 2, 2023

jaekwon Jun 2, 2023 •

edited

Loading

albttx Jun 3, 2023

moul Jun 6, 2023

albttx Jun 12, 2023

moul Jun 12, 2023

jaekwon left a comment

r3v4s commented Jun 13, 2023

r3v4s commented Jun 13, 2023

feat: add std.PrevRealm #667

feat: add std.PrevRealm #667

Conversation

albttx commented Mar 28, 2023 • edited Loading

Description

How it's works

How it's works

Examples

User calm realm, that call realms

User call Realm that call only packages

User call Realm that call packages and realm

How has this been tested?

grepsuzette commented Mar 29, 2023

tbruyelle commented Mar 29, 2023 • edited Loading

r3v4s commented Mar 30, 2023

albttx commented Mar 30, 2023

albttx commented Mar 30, 2023

r3v4s commented Mar 30, 2023

piux2 commented Mar 30, 2023

moul commented Mar 31, 2023

tbruyelle commented Mar 31, 2023 • edited Loading

grepsuzette commented Mar 31, 2023

albttx commented Apr 1, 2023

moul left a comment

Choose a reason for hiding this comment

anarcher commented Apr 2, 2023

moul commented Apr 2, 2023

albttx commented Apr 5, 2023

jaekwon commented Jun 2, 2023

jaekwon commented Jun 2, 2023

jaekwon Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

albttx Jun 3, 2023

Choose a reason for hiding this comment

moul Jun 6, 2023

Choose a reason for hiding this comment

albttx Jun 12, 2023

Choose a reason for hiding this comment

moul Jun 12, 2023

Choose a reason for hiding this comment

jaekwon left a comment

Choose a reason for hiding this comment

r3v4s commented Jun 13, 2023

r3v4s commented Jun 13, 2023

albttx commented Mar 28, 2023 •

edited

Loading

tbruyelle commented Mar 29, 2023 •

edited

Loading

tbruyelle commented Mar 31, 2023 •

edited

Loading

jaekwon Jun 2, 2023 •

edited

Loading